-
Notifications
You must be signed in to change notification settings - Fork 432
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Migrate rand_distr to num-traits for no_std support #987
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well spotted that since rand_distr
is now an external crate, there's less pressure to minimise dependencies. But even so (and at risk of being accused of NIH), is there a good rationale for this change?
Aha, because num_traits
provides trig functions not dependent on std
/ libm
.
Looks like we'll need to bump the MSRV to 1.36; that's nearly a year old so probably fine. The latest Debian release only appears to support 1.34 however, so not so sure. But in any case the issue is only that the Not sure what the other failure is. Lets retry later. |
We could use the Probably we should synchronize |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change looks good, especially the no_std
support is something I wanted for a long time. I would like to see the potential undefined behavior from casting f64
to u64
resolved, with or without bringing back the u64
impl for Poisson
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding the MSRV, couldn't we simply change our testing to say the MSRV is 1.34 in general but 1.36 for the alloc
feature? But if it gets complicated, then just bump to 1.36, agreed.
I've used the following workaround to get the old MSRV: #[cfg(all(feature = "alloc", not(feature = "std")))]
extern crate alloc;
#[cfg(feature = "std")]
extern crate std;
// TODO: remove on MSRV bump to 1.36
#[cfg(feature = "std")]
extern crate std as alloc; Also note that I've tweaked Dirichlet API a bit, I can revert the commit if you don't like the changes. |
What's the rationale for this change? |
Boxed slice is smaller than vector and all examples effectively use arrays for initialization (unfortunately I couldn't find real-world uses after a cursory github search), so I think it makes API simpler. Yes, it may result in an unnecessary allocation, but I doubt it will be common in practice. Ideally it should use const generics and I think slice is closer to that than vector, so it could save some amount of churn during potential future migration. |
I just realized that this PR conflicts with another open PR, #958, which introduces support for
Fair enough!
I don't see how this would help with churn, switching to const generics will be a breaking change anyway. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to go to me (with the minor documentation change I suggested).
@dhardy Any concerns?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the most part this looks good.
Do you intend to squash or rebase this? It does several different things (such as moving value stability tests and the num-traits migration) which is quite a lot for one commit, but it is not desirable to keep the little fixes separate.
Personally, I prefer squashing PRs. |
I believe this can be merged after squashing and rebasing? |
Yes. Does it need a rebase or just a squash? (I still think it's too much content for one commit, but nvm.) |
I think simply pressing the "Squash and merge" button should be enough. |
Closes: #922
If I am not mistaken, reproducibility tests on x86-64 pass without issues.